Repository navigation
[3/8] Preserve MGS1 configuration and Integral language choices - #115
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38bc200492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // read itself follows this hook on the same thread, so the two agree. | ||
| if (M2Config::bPatchesPreserveConfiguration && MGS1_ConfigPTR != 0 | ||
| && width == 16 && offset == MGS1_ConfigPTR) { | ||
| MGS1_ConfigSeen = SQEmuTask<Squirk::Standard>::GetRamValue(16, MGS1_ConfigPTR) & 0xFFFF; |
There was a problem hiding this comment.
With the default PreserveConfiguration enabled, a collection call to getRamValue(16, MGS1_ConfigPTR) enters SQNative_getRamValue, which calls this callback; this line then invokes the same hooked getRamValue again before the callback can return. Every nested invocation satisfies the same condition, causing unbounded recursion and a stack overflow instead of capturing the configuration. Read through an unhooked path or add a reentrancy guard around the internal read.
Useful? React with 👍 / 👎.
nuggslet
left a comment
nuggslet
left a comment
There was a problem hiding this comment.
LGTM - weird .ini newline padding but I'll adjust it if I'm not keen :P
Collection initialization could reset MGS1 configuration or Integral language choices. Preserve requested configuration and apply the English-text language choice at the relevant game hooks. Add the required RAM hooks and configuration documentation.
Review order
PR 3 of 8 in the complete TideGear contribution. These are an ordered stack against upstream
master; GitHub shows earlier unmerged changes cumulatively. Merge in order 1 through 8 and update the remaining branches after each merge.Previous PR: #114
Review only this step
Validation
Ready for maintainer review. The documented gameplay testing remains outstanding; no gameplay pass is claimed.
Complete series